Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added order_by() to SqlaTable to ensure alphabetically ordered filter options values. #4712

Closed
wants to merge 6 commits into from

Conversation

JeppeSigaard
Copy link

@JeppeSigaard JeppeSigaard commented Mar 29, 2018

Added order_by(column_name) to SqlaTable model to ensure correctly ordered value choices in filter controls.

@mistercrunch
Copy link
Member

I feel like this should be done on the backend. From my experience it's been sorted, I'm guessing the SELECT DISTINCT will often do this, though it's not guaranteed. What backend do you use?

@mistercrunch
Copy link
Member

We should add an .order here:
https://github.com/apache/incubator-superset/blob/master/superset/connectors/sqla/models.py#L396

and get the database to do the work.

Added order_by to SqlaTable model
@JeppeSigaard
Copy link
Author

JeppeSigaard commented Apr 11, 2018

Well, in terms of database in use, I just popped open the dev setup. Non-alphabetical values can be spotted by setting up a dev environment using the test data as per the documentation.
A good example would be the response of http://localhost:8088/superset/filter/table/2/country_code/, where "DZA" is popped in there with the A's.

I rolled back the client side array sort and instead added an order_by() as suggested.

This is also a valid solution 👍

@JeppeSigaard JeppeSigaard changed the title Added localecompare array sort to datahandler, fetchFilterValues Added order_by() to SqlaTable to ensure alphabetically ordered filter options values. Apr 11, 2018
@codecov-io
Copy link

codecov-io commented Apr 11, 2018

Codecov Report

Merging #4712 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #4712   +/-   ##
=======================================
  Coverage   76.96%   76.96%           
=======================================
  Files          44       44           
  Lines        8534     8534           
=======================================
  Hits         6568     6568           
  Misses       1966     1966
Impacted Files Coverage Δ
superset/connectors/sqla/models.py 74.23% <ø> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 17ae9ec...2df808a. Read the comment docs.

@stale
Copy link

stale bot commented Apr 10, 2019

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the inactive Inactive for >= 30 days label Apr 10, 2019
@stale stale bot closed this Apr 17, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
inactive Inactive for >= 30 days
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants